PromQL: Wire WITHOUT in Translator by sidosera · Pull Request #144261 · elastic/elasticsearch

sidosera · 2026-03-14T02:36:58Z

Adds the translation flow for PromQL without grouping as part of #139793.

Summary:

Builds on PromQL: Plumbing for WITHOUT Support #143269, which contains the prep-only plumbing and gated coverage.
Wires WITHOUT through translator pipline (selector translation, aggregate planning, etc)
Review tip: the prep commit is already covered by PromQL: Plumbing for WITHOUT Support #143269; the new behavior here is the top commit in this branch.

Stack:

elasticsearchmachine · 2026-03-19T00:00:04Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

felixbarny

I'm not though the PR yet but some early comments here. This PR is now also quite large again.

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

felixbarny · 2026-03-19T07:56:40Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

+        // For WITHOUT the concrete labels are only determined during translation (they depend on which labels
+        // the data actually has minus the excluded set), so we must resolve them against the translated plan.
+        if (result.hasExcludedLabels()) {
+            plan = new Project(promqlCommand.source(), plan, resolveOutput(promqlCommand, plan, result));


Resolution is an overloaded term here. The analyzer has a resolution phase where it resolves tables/functions/attributes etc. We should probably use a different term here.

felixbarny · 2026-03-19T07:57:13Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

        return Literal.timeDuration(promqlCommand.source(), Duration.ofMillis(Math.max(1L, nextRoundedValue - roundedStart)));
    }

+    /**


For most of these helper methods, there's potential to simplify by using the streams API rather than manual/imperative loops and filtering.

felixbarny · 2026-03-19T08:10:32Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

+        projections.add(stepAttr);

-        return new Aggregate(promqlCommand.source(), plan, groupings, aggs);
+        List<Attribute> expectedExtra = promqlCommand.output().subList(2, promqlCommand.output().size());


expectedExtra sounds a bit cryptic. These are the dimension columns, right? Maybe add a short method-level comment that describes the column layout like [value, step, ...dimensions].

felixbarny · 2026-03-19T08:13:00Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

+        return new PackedGrouping(new Project(ctx.promqlCommand().source(), rewritten, projections), packedAttr);
+    }
+
+    private static List<NamedExpression> resolveOutput(PromqlCommand promqlCommand, LogicalPlan plan, TranslationResult result) {


A high level description of what this does and what the purpose is would be useful.

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

felixbarny · 2026-03-19T15:46:35Z

...esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/promql/PromqlEsqlCommandTests.java

        assertThat(tsAggregate.timeBucket().buckets().fold(FoldContext.small()), equalTo(Duration.ofMinutes(10)));
    }
+
+    public void testWithoutGroupingProducesTimeSeriesOutput() {


These tests can be removed now, right? They live in PromqlPlanWithoutGroupingTests now.

felixbarny

The logic is sound but the code reads as a sequence of "what" rather than "why" and I found it hard to follow, even though I have an understanding what this is trying to do at a high level. The biggest single improvement would be a concise top-level explanation of the label-propagation model, followed by splitting createOuterAggregate into its two distinct cases. Maybe there are more things we can do to make it more readable.

felixbarny · 2026-03-23T10:26:01Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

@@ -217,20 +267,56 @@ private TranslationResult translateNode(LogicalPlan node, LogicalPlan currentPla
     * expressions embedded inside the aggregation, but do not create Aggregate plan nodes themselves.
     */
    private TranslationResult translateAcrossSeriesAggregate(AcrossSeriesAggregate agg, LogicalPlan currentPlan, TranslationContext ctx) {


The whole PR is built around a bidirectional label-propagation scheme — requiredLabels flows down, exposedLabels/excludedLabels flow up — but this model is never stated explicitly. A developer encountering this code cold has to reverse-engineer it from the individual pieces. A single Javadoc comment at the top of translateAcrossSeriesAggregate explaining the two-pass propagation strategy and why it's needed for WITHOUT would be worth more than all the inline comments combined.

Good suggestion. Added doc section for that.

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

felixbarny · 2026-03-23T10:44:52Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

+                }
+            }
+
+            // TODO: Should we fail in case of missmatch?


Suggested change

// TODO: Should we fail in case of missmatch?

// TODO: Should we fail in case of mismatch?

In what cases does a mismatch happen? Can we validate this early and throw a validation error (4xx) rather than failing here (5xx)?

We should probably surface this in some form, but it is not straightforward as far as I can tell because PromQL translation happens after analysis. For example, in:

avg by (c) ( sum without (b) ( max by (a,b) (...) ) )

we can tell that the result is meaningless because c cannot appear in the output of the without stage since innermost doesn't produce it. This is likely typo and warning would be a good UX (not necessary fail query if we want to stay compliant with PromQL spec).

Let's track an issue for this. Also remember that 5xx will cause serverless alerts.

felixbarny · 2026-03-23T10:47:48Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

+                resolved.add(byName);
+            }
+        }
+        return normalizeLabels(resolved);


Nit: Some callers pass in the output of prior `normalizeLabels(...) calls. This is low overhead but slightly redundant.

felixbarny · 2026-03-23T11:07:08Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

+    /**
+     * Strips non-grouping attributes (metadata, unresolved, metrics, NULLs) and deduplicates by field name.
+     */
+    private static List<Attribute> normalizeLabels(List<Attribute> attributes) {


Calling this for agg.output() seems redundant as nulls should be filtered out already. For agg.groupings() it seems we don't need the full normalization but just the null filtering?

The only place where all of the normalization is needed is normalizeLabels(resolved) inside resolveLabels(). Maybe also rename to something less vague than "normalize". Maybe something like retainLabelAttributes.

sidosera · 2026-03-26T02:26:23Z

The logic is sound but the code reads as a sequence of "what" rather than "why" and I found it hard to follow, even though I have an understanding what this is trying to do at a high level. The biggest single improvement would be a concise top-level explanation of the label-propagation model, followed by splitting createOuterAggregate into its two distinct cases. Maybe there are more things we can do to make it more readable.

Thanks for the feedback @felixbarny. I agree, we may want to add diagrams and examples to illustrate label propagation model. I took a pass and added one high level summary above createOuterAggregate function. I also addressed your broader concern wrt readability in follow-up PR #144261. LMK if this makes sense now.

felixbarny · 2026-03-26T12:09:03Z

...a/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToEsqlPlan.java

+                }
+            }
+
+            // TODO: Should we fail in case of missmatch?


Let's track an issue for this. Also remember that 5xx will cause serverless alerts.

Stage the capability flag, lowering prep, and gated coverage needed for incremental WITHOUT support while keeping translator behavior unchanged.

Wire WITHOUT grouping through selector translation, aggregate planning, and root projection so the staged plumbing becomes functional. Enable the capability and focused verifier/optimizer coverage for the supported WITHOUT cases.

Upstream moved STEP_COLUMN_NAME to PromqlCommand; use the accessor.

jeramysoucy · 2026-03-26T15:28:49Z

@sidosera It doesn't look like Kibana Security owns any changes here though we were triggered as code owner. I will remove the request for our team, but I am still happy to review if there is something you'd like us to look at.

sidosera · 2026-03-27T12:51:42Z

Closing this as future refactoring PR landed containing this change.

sidosera mentioned this pull request Mar 14, 2026

PromQL: Plumbing for WITHOUT Support #143269

Merged

elasticsearchmachine added v9.4.0 needs:triage Requires assignment of a team area label labels Mar 14, 2026

sidosera marked this pull request as draft March 14, 2026 02:42

sidosera added Team:StorageEngine :StorageEngine/PromQL PromQL support for Elastic >non-issue and removed needs:triage Requires assignment of a team area label labels Mar 14, 2026

sidosera changed the title ~~PromQL: Wire WITHOUT translation flow~~ PromQL: Wire WITHOUT in Translator Mar 14, 2026

sidosera requested a review from felixbarny March 14, 2026 03:22

sidosera marked this pull request as ready for review March 18, 2026 23:59

felixbarny reviewed Mar 19, 2026

View reviewed changes

felixbarny reviewed Mar 23, 2026

View reviewed changes

sidosera mentioned this pull request Mar 25, 2026

[PromQL] PromQL Translator Refactor #144953

Merged

sidosera requested a review from felixbarny March 26, 2026 10:49

felixbarny approved these changes Mar 26, 2026

View reviewed changes

sidosera added 4 commits March 26, 2026 14:56

Prepare PromQL WITHOUT plumbing

3376dd4

Stage the capability flag, lowering prep, and gated coverage needed for incremental WITHOUT support while keeping translator behavior unchanged.

Add PromQL WITHOUT translation flow

f9e182b

Wire WITHOUT grouping through selector translation, aggregate planning, and root projection so the staged plumbing becomes functional. Enable the capability and focused verifier/optimizer coverage for the supported WITHOUT cases.

Address review suggestions

a9da1a9

Use PromqlCommand.stepColumnName() for step lookup fallback

4fd76d4

Upstream moved STEP_COLUMN_NAME to PromqlCommand; use the accessor.

sidosera requested review from a team as code owners March 26, 2026 15:13

sidosera requested a review from a team as a code owner March 26, 2026 15:13

sidosera requested a review from jeramysoucy March 26, 2026 15:13

sidosera force-pushed the feature/promql-without-grouping-v2 branch from 2350951 to 4fd76d4 Compare March 26, 2026 15:17

jeramysoucy removed request for a team and jeramysoucy March 26, 2026 15:29

Remove redundant test cases for PromQL WITHOUT scenarios

2072fa3

sidosera removed request for a team March 26, 2026 17:43

sidosera enabled auto-merge (squash) March 26, 2026 17:43

shainaraskas removed the request for review from a team March 26, 2026 19:52

sidosera added 3 commits March 26, 2026 22:03

Merge branch 'main' into feature/promql-without-grouping-v2

569d89b

Refine comments for PromQL WITHOUT label handling

2b0e2f4

Merge branch 'main' into feature/promql-without-grouping-v2

70ed3f0

sidosera closed this Mar 27, 2026

auto-merge was automatically disabled March 27, 2026 12:51
Pull request was closed

	// TODO: Should we fail in case of missmatch?
	// TODO: Should we fail in case of mismatch?

Conversation

sidosera commented Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 19, 2026

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidosera commented Mar 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeramysoucy commented Mar 26, 2026

Uh oh!

sidosera commented Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sidosera commented Mar 14, 2026 •

edited

Loading